api: add ordered header mutations to ClientTrafficPolicy header filter - #9555
api: add ordered header mutations to ClientTrafficPolicy header filter#9555wbpcode wants to merge 2 commits into
Conversation
Signed-off-by: wbpcode <wbphub@gmail.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cd26c73bd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // +optional | ||
| // +kubebuilder:validation:MaxItems=64 | ||
| Mutations []HTTPHeaderMutation `json:"mutations,omitempty"` |
There was a problem hiding this comment.
When a ClientTrafficPolicy uses earlyRequestHeaders or lateResponseHeaders with only the new mutations field, the CRD accepts it (and the added CEL test treats it as valid), but the controller still calls translateHeaderModifier, which only reads Add, Set, AddIfAbsent, Remove, and RemoveOnMatch and then reports did not provide valid configuration when those legacy slices are empty (internal/gatewayapi/clienttrafficpolicy.go:1339-1512). In that scenario the newly documented ordered operations never reach the IR/xDS and the policy is rejected or has no header effect, so this API needs corresponding translation/status/testdata support before it is exposed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is for API only discussion.
| // Mutations is an ordered list of header operations that are applied in | ||
| // exactly the order specified. Use this field when the sequence of | ||
| // operations matters, for example setting a header and then appending to | ||
| // it, or removing a header and then re-adding it. | ||
| // | ||
| // Mutations are always applied FIRST, in list order. The Set, Add, | ||
| // AddIfAbsent, Remove and RemoveOnMatch fields below are then applied after | ||
| // the mutations, preserving their existing ordering (Add, then Set, then | ||
| // AddIfAbsent, then Remove, then RemoveOnMatch). | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=64 | ||
| Mutations []HTTPHeaderMutation `json:"mutations,omitempty"` |
There was a problem hiding this comment.
This commit exposes a new user-facing ClientTrafficPolicy header-mutation API and updates the generated docs/CRDs, but it does not add any release-notes/current/new_features/... fragment. The repository's release-note workflow expects notable new capabilities and existing API changes to be documented there (release-notes/current/README.md:8-25), so this feature can otherwise ship without appearing in the next release notes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is API only for discussion.
03e2abf to
ac5dc32
Compare
| // Mutations is an ordered list of header operations that are applied in | ||
| // exactly the order specified. Use this field when the sequence of | ||
| // operations matters, for example setting a header and then appending to | ||
| // it, or removing a header and then re-adding it. |
There was a problem hiding this comment.
Thanks for putting this up. Before we settle the shape, the two examples seem not justify the API.
Both examples are already covered by the existing API:
- "set then append" → the result is just a final value: set: {value: "a,b"} does it in one op.
- "remove then re-add" → net effect is "header ends up with exactly my value" — that's the definition of set (OVERWRITE_IF_EXISTS_OR_ADD).
There was a problem hiding this comment.
Hello, @zhaohuabing . Good point. But note, although we may coalesce multiple values with , (only for inline headers). It's not means they have completely same semantics to for x: a,b and x: a x: b. Esp after we enabled the envoy.reloadable_features.match_headers_individually.
"remove then re-add" → net effect is "header ends up with exactly my value" — that's the definition of set (OVERWRITE_IF_EXISTS_OR_ADD).
When I see remove there, I mean to use remove or remove_on_match. It's very possible for a users to remove a list of headers (like all headers with x-custom- prefix) with remove_on_match and then add a specific one x-custom-specific.
3 participants
What this PR does / why we need it:
It's usual requirement to control the order of the header mutations. Like we may want to remove some existing headers and then add new one and so on.
And sometimes the users may also want to keep an empty header value if the substitution formatter failed to get the target value.
All these features are supported by the Envoy data plane. This PR add a new fields to the HTTPHeaderFilter to make the users could configure a list of mutations. And the mutations will be applied in definite order.
Which issue(s) this PR fixes:
Fixes #
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.